Details

    • New Feature
    • Status: Closed
    • Major
    • Resolution: Implemented
    • None
    • None
    • None
    • None

    Description

      In reference to the proposal sent on the developer list, this ticket will be used for tracking the work progress of the SMS gateway integration.

      I have added design proposal for the SMS gateway integration to the wiki and can be accessed from here.

      https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=87296614

      Attachments

        1. OFBIZ-10457.patch
          16 kB
          Pritam Kute
        2. OFBIZ-10457.patch
          17 kB
          Pritam Kute
        3. msg91.zip
          23 kB
          Pritam Kute
        4. mobile-screenshot.jpeg
          23 kB
          Pritam Kute

        Activity

          pritam.kute Pritam Kute added a comment -

          I have attached the first pass of the SMS Gateway Integration. I have tested the integration with Msg91 SMS Gateway.

          The patch contains the generic code/data. All custom code/data corresponding to Msg91 gateway is present in the attached plugin (msg91.zip).

          Please have a look and let me know your feedback/suggestions/improvements.

          pritam.kute Pritam Kute added a comment - I have attached the first pass of the SMS Gateway Integration. I have tested the integration with Msg91 SMS Gateway . The patch contains the generic code/data. All custom code/data corresponding to Msg91 gateway is present in the attached plugin (msg91.zip). Please have a look and let me know your feedback/suggestions/improvements.
          pierresmits Pierre Smits added a comment -

          Having done a cursory review of the artefacts in the attached zip file, I found:

          • that everything references a specific texting service provider (msg91https://msg91.com) instead of using a generic functional name;
          • I could not find whether the service provider (Walkover Web Solutions Pvt. Ltd.) operates in other countries than only India;
          • a new entity is introduced (*Msg91*GatewayConfig),  which seems to be intended to capture (and deliver for the functions) the configuration aspects. This is unnecessary as such parameters can also be in the SystemProperty table.

          Though I did not explore this thoroughly I doubt that this would work with other 3rd party SMS solutions. IMO, in order to have this incorporated in the code base some more effort should be applied to make it more generic and applicable to multiple 3rd party SMS solutions. Otherwise this is too 1party specific.

          pierresmits Pierre Smits added a comment - Having done a cursory review of the artefacts in the attached zip file, I found: that everything references a specific texting service provider ( msg91 -  https://msg91.com)  instead of using a generic functional name; I could not find whether the service provider (Walkover Web Solutions Pvt. Ltd.) operates in other countries than only India; a new entity is introduced (*Msg91*GatewayConfig),  which seems to be intended to capture (and deliver for the functions) the configuration aspects. This is unnecessary as such parameters can also be in the SystemProperty table. Though I did not explore this thoroughly I doubt that this would work with other 3rd party SMS solutions. IMO, in order to have this incorporated in the code base some more effort should be applied to make it more generic and applicable to multiple 3rd party SMS solutions. Otherwise this is too 1party specific.
          mbrohl Michael Brohl added a comment -

          I only had a brief look but my basic understanding of the approach is that we have generic functionality in the framework (configuration, productstore setiings etc.), see patch.

          The specific SMS providers will then be provided as a plugin. In this case, the plugin is specifically for the msg91 provider. This way you only use the plugin for a provider you need.

          If you need another one you'll have to write the integration code for it.

          This looks good to my in the overview (no in depth review from my side yet).

          mbrohl Michael Brohl added a comment - I only had a brief look but my basic understanding of the approach is that we have generic functionality in the framework (configuration, productstore setiings etc.), see patch. The specific SMS providers will then be provided as a plugin. In this case, the plugin is specifically for the msg91 provider. This way you only use the plugin for a provider you need. If you need another one you'll have to write the integration code for it. This looks good to my in the overview (no in depth review from my side yet).
          pritam.kute Pritam Kute added a comment - - edited

          Thanks Michael Brohl. You got it right. The patch contains the generic changes which can be pushed to the codebase. The plugin contains all the specific code need to write to push messages to the gateway server.

          Thanks [~pfm.smits] for your review. Msg91 API can send the SMS all over the world. I haven't tested it but API documentation claims that. I can push some more plugins to show how this generic code can work with popular SMS gateways like Textlocal and Twilio.

          I have used the implementation flow similar to payment gateway integration in OFBiz. The "Msg91GatewayConfig" entity is similar to "PaymentGatewayPayPal" entity.

          pritam.kute Pritam Kute added a comment - - edited Thanks Michael Brohl . You got it right. The patch contains the generic changes which can be pushed to the codebase. The plugin contains all the specific code need to write to push messages to the gateway server. Thanks [~pfm.smits] for your review. Msg91 API can send the SMS all over the world. I haven't tested it but API documentation claims that. I can push some more plugins to show how this generic code can work with popular SMS gateways like Textlocal and Twilio . I have used the implementation flow similar to payment gateway integration in OFBiz. The "Msg91GatewayConfig" entity is similar to "PaymentGatewayPayPal" entity.
          pritam.kute Pritam Kute added a comment -

          I have updated the documentation as per the new design. I have also updated the steps to set up the SMS gateway integration on the local development machine. The link of the document is mentioned in the ticket description.

          pritam.kute Pritam Kute added a comment - I have updated the documentation as per the new design. I have also updated the steps to set up the SMS gateway integration on the local development machine. The link of the document is mentioned in the ticket description.
          rishisolankii Rishi Solanki added a comment -

          I reviewed the work done for the ticket, the basic setup concerns raised by Pierre are good. But the pattern opted in the patch/solution provided also seems to be good and can be consider as solution which is already there in the framework. So I would like to go ahead and commit the changes to have this functionality in the OFBiz.

          In a day or two I'll commit the patch if no objection from community. Please feel free to suggest any improvements should be taken care.

          rishisolankii Rishi Solanki added a comment - I reviewed the work done for the ticket, the basic setup concerns raised by Pierre are good. But the pattern opted in the patch/solution provided also seems to be good and can be consider as solution which is already there in the framework. So I would like to go ahead and commit the changes to have this functionality in the OFBiz. In a day or two I'll commit the patch if no objection from community. Please feel free to suggest any improvements should be taken care.
          mbrohl Michael Brohl added a comment -

          +1

          mbrohl Michael Brohl added a comment - +1
          pritam.kute Pritam Kute added a comment -

          Updated the patch with following improvements:
          1) Added optional "configId" as an input parameter to "sendTelecomMessageInterface".
          2) Added optional "telecomGatewayConfigId" as an input parameter to "sendTelecomMessage" service.
          3) Added "defaultFromTelecomAddress" property to general.properties to feed it to the CommunicationEvent. This will help in identifying the CommunicationEvent for Telecom transactions.
          4) Instead of "fromString", "telecomMsgTypeEnumId" is now pushed to "subject" field in CommunicationEvent to make it more readable.
          5) Added "default-value" to the "telecomMethodTypeId" attribute as "SMS".
          6) Handled the case if ProductStoreTelecomSetting is not found for the given inputs.

          pritam.kute Pritam Kute added a comment - Updated the patch with following improvements: 1) Added optional "configId" as an input parameter to "sendTelecomMessageInterface". 2) Added optional "telecomGatewayConfigId" as an input parameter to "sendTelecomMessage" service. 3) Added "defaultFromTelecomAddress" property to general.properties to feed it to the CommunicationEvent. This will help in identifying the CommunicationEvent for Telecom transactions. 4) Instead of "fromString", "telecomMsgTypeEnumId" is now pushed to "subject" field in CommunicationEvent to make it more readable. 5) Added "default-value" to the "telecomMethodTypeId" attribute as "SMS". 6) Handled the case if ProductStoreTelecomSetting is not found for the given inputs.
          rishisolankii Rishi Solanki added a comment -

          Thanks Pritam Kute for your contribution, your patch has been committed in revision 1855389.

          I haven't committed the message91 as plugin, so we can consider using the base implementation and based on the custom need anyone can add the custom service and configure it via store setting and get it enable.

           

          Community, please let us know if you think we should commit the example plugin as well. Thanks!

          rishisolankii Rishi Solanki added a comment - Thanks Pritam Kute for your contribution, your patch has been committed in revision 1855389. I haven't committed the message91 as plugin, so we can consider using the base implementation and based on the custom need anyone can add the custom service and configure it via store setting and get it enable.   Community, please let us know if you think we should commit the example plugin as well. Thanks!
          rishisolankii Rishi Solanki added a comment -

          Closing the ticket, in case plugin required then will reopen the ticket.

          rishisolankii Rishi Solanki added a comment - Closing the ticket, in case plugin required then will reopen the ticket.
          mbrohl Michael Brohl added a comment -

          Hi Rishi Solanki,

          it think it makes sense to commit the message91 implementation as a reference/example on how to use the base implementation and also to have something to test. The base implementation should also point the user to it.

          Thanks!

          mbrohl Michael Brohl added a comment - Hi Rishi Solanki , it think it makes sense to commit the message91 implementation as a reference/example on how to use the base implementation and also to have something to test. The base implementation should also point the user to it. Thanks!
          rishisolankii Rishi Solanki added a comment - - edited

          Perfect. Thanks Michael Brohl for comment on it. Very soon I will commit the new plugin, need to do some improvements (not major) before that. Thanks again!

          rishisolankii Rishi Solanki added a comment - - edited Perfect. Thanks Michael Brohl for comment on it. Very soon I will commit the new plugin, need to do some improvements (not major) before that. Thanks again!
          rishisolankii Rishi Solanki added a comment -

          Open issue to add msg91 plugin.

          rishisolankii Rishi Solanki added a comment - Open issue to add msg91 plugin.
          rishisolankii Rishi Solanki added a comment -

          Added msg91 plugins with following changes;

          • Surpress warnings coming during build.
          • Removed build.gradle, forms.xml and change the entries in ofbiz-component.xml
          • Added README.md file and Seed data to be used.
          • Added how to run and how to test steps in README.md file.

          Changes done in trunk revision 1856618 and 1856618.

          Thanks Pritam Kute for your contribution.

          Thanks Michael Brohl for review and feedback.

          rishisolankii Rishi Solanki added a comment - Added msg91 plugins with following changes; Surpress warnings coming during build. Removed build.gradle, forms.xml and change the entries in ofbiz-component.xml Added README.md file and Seed data to be used. Added how to run and how to test steps in README.md file. Changes done in trunk revision 1856618 and 1856618. Thanks Pritam Kute for your contribution. Thanks Michael Brohl for review and feedback.

          People

            singh.vivek599 vivek singh bisen
            pritam.kute Pritam Kute
            Votes:
            2 Vote for this issue
            Watchers:
            Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack